Mark empty _terms_enum results due to DLS as incomplete (#91720)#2
Mark empty _terms_enum results due to DLS as incomplete (#91720)#2MitchLewis930 wants to merge 1 commit intopr_012_beforefrom
_terms_enum results due to DLS as incomplete (#91720)#2Conversation
) Today `_terms_enum` returns empty results for indices with document level security. Elasticsearch should return some hint in case the user hits empty results due to DLS limitation so the caller (ie. Kibana) can fall back to other strategies or notify the user with some appropriate error message. This changes the behaviour of the NodeTransportHandler so that it returns a NodeTermsEnumResponse with an error indication. The resulting API response will flag the enum as "incomplete" and list the error in the shard errors section. Clients can choose to react to this in the appropriate way. Closes elastic#88321
📝 WalkthroughWalkthroughThe changes enhance the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/termsenum/action/TransportTermsEnumAction.java`:
- Around line 718-734: In TransportTermsEnumAction (inside the shard iteration
where canAccess(shardId, request, frozenLicenseState, threadContext) is checked)
add an immediate return after calling listener.onResponse(...) so the
method/loop exits and no further responses are sent; also update the nearby
comment above the canMatchShard(shardId, request) check (currently mentioning
"Permission denied") to accurately reflect that this branch only handles index
filter/match failures. Ensure the change is made where the NodeTermsEnumResponse
is constructed and where request.remove(shardId) is invoked so no duplicate
listener responses occur.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/changelog/91720.yamlx-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/termsenum/action/TransportTermsEnumAction.javax-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/terms_enum/10_basic.yml
🔇 Additional comments (5)
docs/changelog/91720.yaml (1)
1-6: LGTM!The changelog entry is well-structured and accurately describes the enhancement. The summary clearly communicates the change, and the issue reference links back to the original feature request.
x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/terms_enum/10_basic.yml (4)
551-561: Test assertions correctly validate the new DLS restriction behavior.The updated assertions properly verify that when a user with partial DLS access (
dls_some_user) attempts to enumerate terms:
- Empty results are returned (
terms: 0)- The response is marked as incomplete (
complete: false)- A shard failure is reported with the expected error message
This aligns well with the implementation changes in
TransportTermsEnumAction.java.
563-570: FLS behavior correctly differs from DLS behavior.The
fls_usertest correctly expectscomplete: trueand_shards.failed: 0because Field Level Security (FLS) works differently from Document Level Security (DLS). FLS restricts field visibility rather than document access, so the operation completes successfully but returns no terms because thefoofield is excluded from the user's view.
622-633: API key test assertions are consistent with the new behavior.The test correctly verifies that an API key with restrictive DLS permissions (
dls_all_user_bad_key) receives incomplete results with the expected shard failure.
654-664: Bounded API key permissions test is correctly updated.The test validates that even when an API key has permissive DLS settings, it remains bounded by the owner user's (
dls_some_user) restrictions, resulting in incomplete results with shard failures.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if (canAccess(shardId, request, frozenLicenseState, threadContext) == false) { | ||
| listener.onResponse( | ||
| new NodeTermsEnumResponse( | ||
| request.nodeId(), | ||
| Collections.emptyList(), | ||
| "cannot execute [_terms_enum] request on index [" | ||
| + shardId.getIndexName() | ||
| + "] due to " | ||
| + "DLS/FLS security restrictions.", | ||
| false | ||
| ) | ||
| ); | ||
| } | ||
| if (canMatchShard(shardId, request) == false) { | ||
| // Permission denied or can't match, remove shardID from request | ||
| request.remove(shardId); | ||
| } |
There was a problem hiding this comment.
Critical: Missing return after early failure response causes undefined behavior.
When canAccess() returns false, the code sends a response to the listener but does not exit the loop or method. Execution continues to canMatchShard() and potentially sends additional responses. This leads to:
- Multiple responses sent to the same
ActionListener - If multiple shards fail access checks, multiple error responses are sent
- After the loop, another response may be sent at line 737 or via the executor at line 746
🐛 Proposed fix: Add return statement after early failure response
if (canAccess(shardId, request, frozenLicenseState, threadContext) == false) {
listener.onResponse(
new NodeTermsEnumResponse(
request.nodeId(),
Collections.emptyList(),
"cannot execute [_terms_enum] request on index ["
+ shardId.getIndexName()
+ "] due to "
+ "DLS/FLS security restrictions.",
false
)
);
+ return;
}
if (canMatchShard(shardId, request) == false) {
- // Permission denied or can't match, remove shardID from request
+ // Can't match index filter, remove shardID from request
request.remove(shardId);
}Additionally, the comment on line 732 mentions "Permission denied" but that condition is now handled separately above. Consider updating the comment to only reference the index filter check.
🤖 Prompt for AI Agents
In
`@x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/termsenum/action/TransportTermsEnumAction.java`
around lines 718 - 734, In TransportTermsEnumAction (inside the shard iteration
where canAccess(shardId, request, frozenLicenseState, threadContext) is checked)
add an immediate return after calling listener.onResponse(...) so the
method/loop exits and no further responses are sent; also update the nearby
comment above the canMatchShard(shardId, request) check (currently mentioning
"Permission denied") to accurately reflect that this branch only handles index
filter/match failures. Ensure the change is made where the NodeTermsEnumResponse
is constructed and where request.remove(shardId) is invoked so no duplicate
listener responses occur.
1 participant
PR_012
Summary by CodeRabbit
Enhancements
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.